-
-
Notifications
You must be signed in to change notification settings - Fork 260
WIP: Add MANY type annotations 🚀 #1396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hey bew, this sounds amazing, I'm happy you got nerd-sniped like that :P It's pretty late for me right now, but I'll take a look at this first thing tomorrow and just answer the few things I can without looking deeper into your changes :)
Oh, I totally agree here, the decision to make all of those the same thing was just because it was easy to incrementally cram more stuff into that one lua-table, but if I were to make a top-down decision right now, those'd have to be separated.
Having a split there sounds pretty nice, is it possible to do so via LuaCATS?
Oh, I think that's a good idea, this is also something I've first written in 1048cdf (or even earlier 👀), more than four years ago, and not really questioned since 😅
Nah, I'll look into merging it, that band-aid will have to come off anyway, this is a good reason to finally do that. Speaking a bit more on changes to luasnips' internals: while self-dependent-dNode already contains a few larger changes, I plan on making more drastic ones, up to a point where we'd (internally) only have
I'd be happy to handle this similarly to the last PR we did, that was very pleasant :D |
|
Take your time! 🙏 Really happy that you'd be okay with making some structural changes! ✨
Hmm actually maybe not with LuaCATS alone 👀
Oh yeah 🚀
I personally like having types when refactoring, because the LSP is smarter and can immediately show you where things are used and how without having to hold everything in your head, and can show the impact of changes with diagnostics.. For the review, I think the most important ones are in |
Always :P
Puuuh, that does not sound appealing, nodes have like >20 fields/functions that are not supposed to be used by users. tmp = self.fn(
effective_args,
self.parent ---[[@as LuaSnip.PublicAPISnippetNode]],
old_state,
unpack(self.user_args)
)
and then
Ah, that's true, I was thinking more of reimplementing the internals from scratch than an incremental change. There are lots and lots of things we don't really need anymore/have better ways of doing (for example
👍 Taking a look now 👀 |
lua/luasnip/extras/init.lua
Outdated
| --- s("extras4", { i(1), t { "", "" }, extras.rep(1) }) | ||
| --- ``` | ||
| --- | ||
| ---@param node_ref LuaSnip.NodeRef a single [Node Reference](#node-reference). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you can put the relative path to the DOC.md in here, so the link also works when viewing the file ;)
([Node Reference](../../../DOC.md#node-reference), +- a .. :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 02cabf1
| local DynamicNode = Node:new() | ||
|
|
||
| ---@class LuaSnip.Opts.DynamicNode: LuaSnip.Opts.FunctionNode | ||
| ---@field snippetstring_args? boolean (FIXME: not documented?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, snippetstring_args is a recent addition in self-dependent-dNode: it's a boolean which, when set, makes the dynamicNode pass snippetStrings instead of string[] as the first argument (snippetStrings are text copied from the buffer, with snippets preserved, so you can do stuff like :gsub on a nested snippet)
A bit of a problem with that the signature of LuaSnip.DynamicNode.Fn changes based on that boolean.. Can we properly represent that or can we only do something like args:(string[]|LuaSnip.SnippetString)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it should be possible using @overload I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that sounds like a good substitute 👍
I don't know if it'd even be possible for luals to track which dynamicNodes have snippetstring_args set and which don't, so overload may be as precise as we can get here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the @overload would only help user code, to have the immediately passed callback function have the correct type, but the internal code would have zero knowledge of which node has what (like at runtime).
And the types can still break (show the (opt1|opt2) in callback) if their code is a bit generic functions calling the node functions, and don't want to deal with overloads in their config...
Actually I noticed there is a |
lua/luasnip/nodes/functionNode.lua
Outdated
| ---@alias LuaSnip.FunctionNode.Fn fun(args: (string[])[], parent: LuaSnip.Snippet | LuaSnip.SnippetNode, ...: table): string|string[] | ||
| -- FIXME: how to document each param of the callback function? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good question 👍
I think it's not possible to document free-standing functions like that with LuaCATS :/
For reference:
EmmyLuaLs/emmylua-analyzer-rust#779
LuaLS/lua-language-server#1456
I think the current way of documenting these callbacks in the dynamicNode/functionNode signature is... fine, but not very elegant :/
lua/luasnip/nodes/functionNode.lua
Outdated
| -- FIXME(@bew): The super flexible NodeRef param & the fact that the NodeRef | ||
| -- type is an alias, makes luals make a huge function signature here 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Settings hover.expandAlias in lua-language-server to false makes this work pretty well
Honestly, setting that does not seem like a bad idea at all 👀 The only annoying bit is that it doesn't seem like one can jump to definition from the documentation window :/
(and obviously that not everyone will want to set this only to make this annotation work :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks I added it to my config 👍 (and removed this comment)
👉 Could you maybe add this tip into a kind of editor support section of the DOC ?
(near the other one about completion 🤔 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhmm, sounds reasonable 👍
Oh, yeah that sounds good!
True, that would work.. I guess what makes me hesitate is that almost everything is internal xD |
lua/luasnip/nodes/insertNode.lua
Outdated
| --- FIXME(@bew): how to put the whole InsertNode documentation here, how to deal | ||
| --- with the gifs here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can omit the gifs, they can be added via code in DOC.md (or just by putting them into a separate section that gives more of an overview of insertNode, compared to this detailed documentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just by putting them into a separate section that gives more of an overview of insertNode, compared to this detailed documentation
I like this idea, to split the reference-level docs from the guide-level docs.
In the meantime I'll do a sort of hybrid but'll add a note for this somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, well put 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the rest of the insert node docs in 1641838
The DOC.md generator will need to be updated but I 'included' the gifs with a line like <demo-gif:SomeID> 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think including this in luals-mdgen should be straightforward, and the syntax looks fine (it's even being highlighted for me, what is it for in regular markdown?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it's even being highlighted for me, what is it for in regular markdown?)
FYI: In markdown it's an explicit link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh that's pretty cool 👍
Check out L3MON4D3/luals-mdgen@f4f134b, it implements almost exactly what you suggested (only <media:SomeID> to make it a bit more generic).
These media-links are ignored if their target is not specified, so we no longer need the whole <panvimdoc-ignore> thing, we can just not set media_mapping when generating the markdown for the vimdoc :)
| } | ||
|
|
||
| ---@class LuaSnip.Snippet: LuaSnip.Addable, LuaSnip.ExpandedSnippet | ||
| ---@class LuaSnip.BareInternalSnippet: LuaSnip.Node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get at what point a snippet is a BareInternalSnippet.. In my mind there is Addable for things that can be put into add_snippets, Expandable for stuff that has :matches and condition and the like, and then there is ExpandedSnippet for when the snippet is expanded.
As far as I remember snippet, dependents_dict, child_snippets, static_text, and indentstr are only used by snippets once they are expanded, maybe ExpandedSnippet is a better place for them? (And maybe it'd be a good idea to initialize them in trigger_expand?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to make something for the return type of _S, which is used in different places for defining different types.
And it's made to have the common fields for all other classes that are derived from it.
It's used as the common base for Snippet & SnippetNode:
---@class LuaSnip.SnippetNode: LuaSnip.BareInternalSnippet, LuaSnip.NormalizedSnippetNodeOpts---@class LuaSnip.Snippet: LuaSnip.BareInternalSnippet, LuaSnip.NormalizedSnippetContext, LuaSnip.NormalizedSnippetOpts, LuaSnip.Addable
Those two don't share all the same fields nor the same parent classes, so we need a kind of base class for all their common fields 🤔
As far as I remember snippet, dependents_dict, child_snippets, static_text, and indentstr are only used by snippets once they are expanded, maybe ExpandedSnippet is a better place for them? (And maybe it'd be a good idea to initialize them in trigger_expand?)
Interesting, I'll have to explore that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to make something for the return type of _S, which is used in different places for defining different types.
And it's made to have the common fields for all other classes that are derived from it.
It's used as the common base for Snippet & SnippetNode
Ahh I see.. this seems fine 👍
So far I'd thought of snippet as a specialization of snippetNode, but it's possible that, rigourously, there are some conflicts there 😅
| -- IDEA(THINKING, @bew): Most methods in Snippet should really be on a BareInternalSnippet class | ||
| -- But this method should be on an actual Snippet class | ||
| -- (so it can be called on a full Snippet, but not a BareInternalSnippet) | ||
| -- This came to my mind because this function uses `self.stored` & | ||
| -- `self.merge_child_ext_opts` that _only_ exist on full Snippet but not on | ||
| -- BareInternalSnippet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Splitting Snippet's responsibilities is a really good idea, it definitely has the weirdest lifecycle out of all nodes.
| -- FIXME(@bew): This should only be on SnippetNode & Snippet 🤔 | ||
| -- (to access callbacks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean this should only be defined on the types used at runtime? I agree :D
|
I rebased on How do you want to handle me fixing things incrementally? |
Perfect, let's hope all goes well 🤞 :D
This sounds great, that way we both know what's been written/verified 👌 |
lua/luasnip/nodes/node.lua
Outdated
| --- | ||
| ---@field visible boolean | ||
| ---@field static_text string[] (FIXME(@bew): Where is this initialized?) | ||
| ---@field static_text string[] (FIXME(@bew): What is this for?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny diff :D
It's used in put_intial, it's the text that will actually appear in the buffer when the snippet is expanded, and it is updated to represent the last text that the user typed into this node (via :store())
|
I also kinda went overboard in 68b7376 & d2b494a as I rewrote the condition objects.. At the moment it's all in this PR (so my config doesn't break 😬), but I'll definitely move those out in a separate PR later on! |
| ---@field show_condition? LuaSnip.SnipContext.ShowConditionFn Same as | ||
| --- `show_condition` in snippet context. (here for backward compat) | ||
| ---@field condition? LuaSnip.SnipContext.Condition Same as `condition` in | ||
| --- snippet context. (here for backward compat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about why these are possible to specify in the snippet opts, is it for backward compat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly, they used to be in that table, and until now I don't think there's been a big reason to actually remove them. Best put something about them being deprecated in there? Unfortunately @deprecated only works for functions 😢
Ah, as opposed to the slightly nebulous and ad-hoc operator-overrides? That sounds good :D
This also sounds great, I guess I'll ignore that file for now, and then do a proper review of it in that separate PR? Definitely chuck your conditions in there, it really can't hurt to have more :) |
| -- FIXME(@bew): What is this for? (not documented..) | ||
| -- FIXME(@bew): Should be moved to its own file? (like the other indexers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know if it is even used anywhere.. it was the first idea for referencing a node that is not a sibling (so no relative jump-index possible).
Checking the code, I can see no references to it, best ignore it for now/rip it out in a separate commit :)
Hola @L3MON4D3, it's been a while o/
I've recently updated my neovim to 0.11 & reworking my config a bit, and since the big PR #1353 that started adding type annotations & DOC.md generation, I didn't touch my snippets at all...
The nvim upgrade was the nudge, and I started replacing the few type annotations I had manually crafted in my config, replacing them with the ones from the plugin!..
...
And then I kinda went down the rabbit hole 🤪
Porting documentation, reading the codebase while adding a few types for the things I understood, and adding MOAR types....
👉 So here is a first draft of ~1k LoC of doc additions & a few small reworks 🚀🚀
It took a few iterations of codebase exploration to kinda understand the various relationships between the different kind of snippet, node, and what 'layer'/'stage' of the snippets definition process I was on
I'm still far from understanding everything, and I might have made some mistakes, put some fields in the wrong class or failed a few inheritance relations, but I think it's pretty good for a start!
There are still many things I saw and could work on / refactor with your permissions, but I got a
good chunk going on and I'm quite happy with it!
Note
I left a few
FIXME/IDEAin the PR with questions for youWarning
I didn't test the DOC.md generation at all, I only focused on the Lua-side of things
Some things I typed/documented
Node, InsertNode, FunctionNode, DynamicNode, ChoiceNode, with (most of) their internal fields ✨
Snippet, SnippetNode & most of the internal snippet-initialization functions (their input opts & their normalized types)
I managed to build a sort of hierarchy of classes that when combined build the final Snippet & SnippetNode types (with extra fields if needed), I'd need careful review of this part, I tried to follow the calls and build the structures but I might be wrong here and there..
I know there are a few fields missing but they're kind of overwelming by their number, and the
lack of internal documentation in such a big codebase 😅
MultiSnippet, SnippetString
some util functions here and there
fmt's main functions
AbsoluteIndexer, KeyIndexer, NodeRef
Other things
copy3impl to util, to avoid duplication betweensnippet.lua&fmt.luaevents.lua&types.luafor nicer typing without duplication👉 In the end, most core files type-checks correctly, with only a few warnings that make sense and
that would need to be addressed (some of them challenge the class hierarchy in place, and I made a
few comments on ideas to improve the situation)
As for where to go with the types design, I think the codebase would benefit from splitting the Snippet class into multiple classes to reflect the various stages of use (barebone for internal use, snippetnode, snippet, expandedsnippet, (..?)). For the actual type annotations, maybe you'd like to have a public/private interface to avoid exposing the huge internals of the nodes to the users 🤔
I was also quite surprised that you used
Node:newboth to create new node types & new node instances, would you welcome some changes around this to segregate the types/functions more by use-cases?(this can probably come later though)
Note
This PR targets the
self-dependent-dNodebranch, since this is the branch I'm on normally.❔ I'm wondering, do you think we could merge it soon or do you still have planned work on it?
I'm asking because otherwise I'd need to rework some parts of this PR to be able to rebase on
master👀Let me know what you think, and how you'd want to tackle this!
I'm available to do a day/night pair-programming/review session if you want 🙃